Tap on Backgrounds for DateEvents and AllDayEvents #20

raLaaaa · 2020-06-21T13:02:24Z

Closes: #18

I thought I'd issue a pull request at the current state regarding #18.

Background taps are currently recognized across DateEvents and AllDayEvents. Tapping on headers is currently not implemented yet.
The pull request adapted the example with a snackbar function.

There are some things on my mind and I'm looking forward to your feedback:

Is the name onCreateEvent and onCreateAllDayEvent really appliceable? Maybe we should change it to somehting like onBackgroundTap or onCellTap?
Regarding DateEvents I'm not quite sure why when calculating the position of the tapped cell I need to add a +1 here:

final dateAndTime = DateTime(date.year, date.monthOfYear, date.dayOfYear, tappedCell.toInt() + 1);

For allday events I even had to do +2 for the day.

final dateAndTime = DateTime(date.year, date.monthOfYear, date.dayOfYear + 2);

I'm not really sure about the calculations at all. The offset issue with the days might be related to wrong rounding? Might also be a conversion problem related to final startTime = LocalDateTime.dateTime(dateAndTime) I think? Maybe you could shed some light into this and the calculation at all. Also the time of the giving back object is always 23:00:00 eventhough it should be 00:00:00

I think there are still some parts where I could make the code partly more elegant. Advices are appreciated :) !

…round

JonasWanke

Sorry for the late reply, and thanks already for your work! I've added some comments (sorry for the amount; some of it is just about formatting).

I like onBackgroundTap, which makes clear that you can also use that listener for other purposes. We can probably merge both parameters of the Timetable widget into a single one, which was the idea behind the additional boolean parameter (true if the tap was in the header area, false if the tap occurred in the part-day area).
I think I found the reason for the + 1: It might be from the conversion between LocalDateTime from time_machine and Dart's DateTime: DateTime by default uses the local timezone, whereas LocalDateTime has no knowledge about timezones at all. See my inline comment for an idea of how to fix it.
It sounds like a conversion problem as well, which would also explain the 23:00:00. See my inline comment for more details.
That would be the comments about extracting lambdas into private helper methods, but otherwise, your code looks good!

lib/src/content/multi_date_content.dart

lib/src/content/timetable_content.dart

lib/src/header/all_day_events.dart

…null

raLaaaa · 2020-06-22T11:47:54Z

Thanks for your detailed feedback Jonas.
I tried to implement all your hints.

The +1 and +2 offset indeed got caught by time_machine and flutters timeformat. Using time_machine directly fixes it.
I increased tap precision. I did not use TimeConstants.millisecondsPerDay though. See

3a59221

From my feeling I'd say there is a more elegant way. I'm not quite sure about this but it works for now.

Is this implementation of attaching the listener only when it's not null legit?

  onTapUp: widget.onEventBackgroundTap != null
                    ? (details) {
                       ...
                      }
                    : null,

We can probably merge both parameters of the Timetable widget into a single one, which was the idea behind the additional boolean parameter (true if the tap was in the header area, false if the tap occurred in the part-day area).

This still needs to be done. Otherwise I think we are nearly done? :)

I'd seperate the tapping inside the header function to a seperate issue and not include in this pull request. Even though it's related I think I'd create a different issue for semantical reasons.

JonasWanke

Is there a reason you didn't use millisecond precision? That would make the code much cleaner:

final millis = details.localPosition.dy / constraints.maxHeight
    * TimeConstants.millisecondsPerDay;
final time = LocalTime.sinceMidnight(Time(milliseconds: millis.floor()));
// Note: I haven't tested this code

Also, users of this package can decide the necessary precision by themselves.

Yes, that's legit. That way, we can also drop the if (widget.onEventBackgroundTap != null) guard (and that in all_day_events.dart) as the listener cannot be null when it should be invoked.
By extracting the private helper function, I meant to extract the whole lambda, though. That way, there isn't an additional ≈ 10 lines of logic inside the already nested build method whose primary purpose is to create the UI. You can simply move the whole lambda content to the helper methods you already created.
I think we might be talking about different "header" functions. My idea was that the callbacks currently called onAllDayEventBackgroundTap and onEventBackgroundTap can be merged into a single callback onEventBackgroundTap. If isAllDay is set to false when that callback is invoked, the user tapped somewhere in the part-day content area. If isAllDay is true, the user tapped in the all-day event area of the header. Sorry for the bad wording :/
Also, I just noticed that the OnCreateEventCallback-typedef can now be renamed to something like OnEventBackgroundTapCallback.

But apart from these (now relatively minor) comments, we're done :)

JonasWanke · 2020-06-23T16:22:39Z

lib/src/content/timetable_content.dart

@@ -55,7 +55,7 @@ class TimetableContent<E extends Event> extends StatelessWidget {
            child: MultiDateContent<E>(
              controller: controller,
              eventBuilder: eventBuilder,
-              onCreateEvent: onCreateEvent
+              onEventBackgroundTap: onEventBackgroundTap


Suggested change

onEventBackgroundTap: onEventBackgroundTap

onEventBackgroundTap: onEventBackgroundTap,

lib/src/header/all_day_events.dart

raLaaaa · 2020-06-24T07:57:36Z

Great feedback. The code is much cleaner now.

I did not use the millisecond method because I could somehow not wrap my hand around even though it's super simple and easy.
Extracting the entire lambda into a helper function definetly makes more sense and makes to code much cleaner.
I merged the two listeners into one now. Thanks for clarifying! What I was thinking about when talking about listener for header was another seperated listener which allows us do receive taps from the headline. See my image:

EG. when tapping where the red arrow points at.

Merging two listeners into one eases the usement of the librabry and makes the code much cleaner. I'm still learning so thanks for your awesome feedback so far! Really appreciated.

JonasWanke · 2020-06-25T06:08:14Z

Now that everything works as intended and the code looks fine, unless you have anything more you want to add, we can merge this PR!

raLaaaa · 2020-06-25T07:55:31Z

Now that everything works as intended and the code looks fine, unless you have anything more you want to add, we can merge this PR!

I think we are good to go :) Cheers!

JonasWanke · 2020-06-25T16:13:22Z

This is now published as part of v0.2.4. Thanks for contributing!

raLaaaa · 2020-06-26T12:01:51Z

Thanks for the opportunity @JonasWanke . Hopefully I can contribute some more in the future!

raLaaaa added 7 commits June 18, 2020 11:20

started working on tapping on background

1490750

dateevents are now tappeable. -WIP

41a824a

WIP - started working on calculating the tapped position in the backg…

a05e4b0

…round

WIP - Fixed offset problem

366fb34

added null checks and snackbar test

81b996a

started working on header taps - WIP

73f2c04

added background tap support for all day events

c567cb7

JonasWanke reviewed Jun 22, 2020

View reviewed changes

JonasWanke assigned raLaaaa Jun 22, 2020

JonasWanke added C: Example Component: Example app C: Timetable Component: The actual timetable package T: Feature Type: :tada: New Features labels Jun 22, 2020

raLaaaa added 4 commits June 22, 2020 10:41

added traling commas and reformatted multi_date_content

f4cf0e7

moved lambda to helper function and only assigning listener when not …

d881fbf

…null

fixed a conversion issue between timemachine and datetime

e40ebd7

minor code refactor and increased tap precision

3a59221

JonasWanke reviewed Jun 23, 2020

View reviewed changes

raLaaaa added 3 commits June 24, 2020 09:26

changed tapup precision and extracted entire lambda to helper function

e7d0b95

extracted entire lambda to helper function for all day events

9ee71d4

refactored seperated listeners into one

8277e69

JonasWanke marked this pull request as ready for review June 25, 2020 06:08

JonasWanke approved these changes Jun 25, 2020

View reviewed changes

JonasWanke mentioned this pull request Jun 25, 2020

TimetableController's scrollController does not update its page value #19

Closed

JonasWanke merged commit cdc7a86 into JonasWanke:master Jun 25, 2020

JonasWanke changed the title ~~WIP: Tap on Backgrounds for DateEvents and AllDayEvents (draft)~~ Tap on Backgrounds for DateEvents and AllDayEvents Jun 25, 2020

abel-123 mentioned this pull request Sep 27, 2022

_RenderWeekIndicatorText does not meet its constraints #131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tap on Backgrounds for DateEvents and AllDayEvents #20

Tap on Backgrounds for DateEvents and AllDayEvents #20

raLaaaa commented Jun 21, 2020 •

edited by JonasWanke

Loading

JonasWanke left a comment

raLaaaa commented Jun 22, 2020 •

edited

Loading

JonasWanke left a comment

JonasWanke Jun 23, 2020

raLaaaa commented Jun 24, 2020

JonasWanke commented Jun 25, 2020

raLaaaa commented Jun 25, 2020

JonasWanke commented Jun 25, 2020

raLaaaa commented Jun 26, 2020

	onEventBackgroundTap: onEventBackgroundTap
	onEventBackgroundTap: onEventBackgroundTap,

Tap on Backgrounds for DateEvents and AllDayEvents #20

Tap on Backgrounds for DateEvents and AllDayEvents #20

Conversation

raLaaaa commented Jun 21, 2020 • edited by JonasWanke Loading

JonasWanke left a comment

Choose a reason for hiding this comment

raLaaaa commented Jun 22, 2020 • edited Loading

JonasWanke left a comment

Choose a reason for hiding this comment

JonasWanke Jun 23, 2020

Choose a reason for hiding this comment

raLaaaa commented Jun 24, 2020

JonasWanke commented Jun 25, 2020

raLaaaa commented Jun 25, 2020

JonasWanke commented Jun 25, 2020

raLaaaa commented Jun 26, 2020

raLaaaa commented Jun 21, 2020 •

edited by JonasWanke

Loading

raLaaaa commented Jun 22, 2020 •

edited

Loading